Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consider invalid entries in the marker file reason for refetch #23336

Closed
wants to merge 3 commits into from
Closed

Consider invalid entries in the marker file reason for refetch #23336

wants to merge 3 commits into from
+134 −20

Conversation

Wyverald
Copy link
Member

@Wyverald Wyverald commented Aug 19, 2024 •
edited
Loading

Especially due to #23127, older marker files may contain entries that don't even parse correctly. Instead of crashing Bazel, we should signal a refetch.

Fixes #23322.

@Wyverald Wyverald requested review from meteorcloudy and fmeum August 19, 2024 18:41
@Wyverald Wyverald requested a review from lberki as a code owner August 19, 2024 18:41
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Aug 19, 2024
@Wyverald
Copy link
Member Author

Note that arguably the correct thing to do here is to always refetch instead of ignoring invalid entries -- although I'm not sure how much trouble we should go through for this corner case.

@Wyverald
Ignore invalid entries in the marker file
0a6880d 
Especially due to #23127, older marker files may contain entries that don't even parse correctly. Instead of crashing Bazel, we should just ignore such entries.

Fixes #23322.
@fmeum
Copy link
Collaborator

fmeum commented Aug 19, 2024

Note that arguably the correct thing to do here is to always refetch instead of ignoring invalid entries -- although I'm not sure how much trouble we should go through for this corner case.

I would prefer that. If it saves us from debugging just one future incrementality issue with repo rules, it will have been worth it.

@iancha1992 iancha1992 added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Aug 19, 2024
@Wyverald
Copy link
Member Author

Fair point! Updated.

@sgowroji sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Aug 21, 2024
@Wyverald Wyverald changed the title Ignore invalid entries in the marker file Consider invalid entries in the marker file reason for refetch Aug 21, 2024
@Wyverald Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Aug 21, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 22, 2024
@Wyverald Wyverald deleted the wyv-fix-marker branch August 22, 2024 19:10
@moroten
Copy link
Contributor

moroten commented Sep 10, 2024

I ran some tests before I did see this fix, appending FILE:@bad#+~//:label foobar to marker files:

8.0.0-pre.20240826.1 is good
8.0.0-pre.20240821.2 is bad
7.3.1 is bad
7.3.0 is bad
7.2.1 is bad
7.1.2 is bad
7.0.2 is good
6.5.0 is good
6.4.0 is good
6.3.2 is good
6.2.1 is good
6.1.2 is good
6.0.0 is good

I've also observed src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java:

        content = FileSystemUtils.readContent(markerPath, UTF_8);
        String markerRuleKey = readMarkerFile(content, recordedInputValues);
        boolean verified = false;
        if (Preconditions.checkNotNull(ruleKey).equals(markerRuleKey)) {
          verified = handler.verifyRecordedInputs(rule, directories, recordedInputValues, env);
          if (env.valuesMissing()) {
            return null;
          }
        }

Because the ruleKey includes a marker version number, I feel like the safest way to be forward compatible is to skip parsing the rest of the marker file if the fingerprint differs. At the moment, all the parser code must be bug free in parsing any future format. @Wyverald What do you think about moving that check into readMarkerFile, passing ruleKey as parameter?

Please cherry-pick d62e0a0 into 7.4.0.
@bazel-io flag

@meteorcloudy
Copy link
Member

@bazel-io fork 7.4.0

@bazel-io bazel-io mentioned this pull request Sep 10, 2024
Closed
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Sep 10, 2024
@Wyverald @bazel-io
Consider invalid entries in the marker file reason for refetch
697fcc5 
Especially due to bazelbuild#23127, older marker files may contain entries that don't even parse correctly. Instead of crashing Bazel, we should signal a refetch.

Fixes bazelbuild#23322.

Closes bazelbuild#23336.

PiperOrigin-RevId: 666416942
Change-Id: Ie5507654f69825d93f9523d9c209d417fb3cdaf6
@bazel-io bazel-io mentioned this pull request Sep 10, 2024
Closed
@Wyverald
Copy link
Member Author

@Wyverald What do you think about moving that check into readMarkerFile, passing ruleKey as parameter?

Yeah this seems fine. If we pass in ruleKey, we can even just get rid of the String return and just return recordedInputValues directly, which is much cleaner.

Please cherry-pick d62e0a0 into 7.4.0.

I don't understand why this is necessary. We don't expect people to manually mess with their marker files.

@moroten
Copy link
Contributor

moroten commented Sep 10, 2024

Please cherry-pick d62e0a0 into 7.4.0.

I don't understand why this is necessary. We don't expect people to manually mess with their marker files.

The reason for cherry-picking is when switching branches. Switching from new branch using a new Bazel version to a branch using 7.1.x and 7.2.x will make Bazel crash if --incompatible_use_plus_in_repo_names was used. In the same way, the risk of crashing Bazel 7.4.x is reduced with this patch, when a future version of Bazel changes the format of the marker files again.

@Wyverald
Copy link
Member Author

I see, that's a valid enough reason.

iancha1992 pushed a commit to bazel-io/bazel that referenced this pull request Sep 12, 2024
@Wyverald @iancha1992
Consider invalid entries in the marker file reason for refetch
cd40eef 
Especially due to bazelbuild#23127, older marker files may contain entries that don't even parse correctly. Instead of crashing Bazel, we should signal a refetch.

Fixes bazelbuild#23322.

Closes bazelbuild#23336.

PiperOrigin-RevId: 666416942
Change-Id: Ie5507654f69825d93f9523d9c209d417fb3cdaf6
moroten added a commit to meroton/bazel that referenced this pull request Sep 17, 2024
@moroten
Make repo marker file parsing robust to version changes
1524409 
Bazel 7.1.0 and 7.2.0 contains a bug where + characters in labels in the
repository marker files cannot be parsed.

Improves bazelbuild#23336 that fixed bazelbuild#23322.
moroten added a commit to meroton/bazel that referenced this pull request Sep 17, 2024
@moroten
Make repo marker file parsing robust to version changes
6164d18 
Bazel 7.1.0 and 7.2.0 contains a bug where + characters in labels in the
repository marker files cannot be parsed. This was fixed in
commit d62e0a0. To reduce the rusk of
future bugs in the same area, this change skips parsing the file if the
first line shows that the content will not be used anyway, which should
be reasonably safe if introducing new formats.

Improves bazelbuild#23336 that fixed bazelbuild#23322.
@moroten moroten mentioned this pull request Sep 17, 2024
Closed
copybara-service bot pushed a commit that referenced this pull request Sep 26, 2024
@moroten @copybara-github
Make repo marker file parsing robust to version changes
c3393da 
Bazel 7.1.0 and 7.2.0 contains a bug where + characters in labels in the repository marker files cannot be parsed. This was fixed in commit d62e0a0. To reduce the rusk of future bugs in the same area, this change skips parsing the file if the first line shows that the content will not be used anyway, which should be reasonably safe if introducing new formats.

Improves #23336 that fixed #23322.

Closes #23642.

PiperOrigin-RevId: 679330823
Change-Id: I8123bdde047735ced3eabed3df68112d44318b08
Wyverald added a commit that referenced this pull request Oct 9, 2024
@Wyverald
Consider invalid entries in the marker file reason for refetch
27b86c9 
Especially due to #23127, older marker files may contain entries that don't even parse correctly. Instead of crashing Bazel, we should signal a refetch.

Fixes #23322.

Closes #23336.

PiperOrigin-RevId: 666416942
Change-Id: Ie5507654f69825d93f9523d9c209d417fb3cdaf6
Wyverald pushed a commit that referenced this pull request Oct 9, 2024
@moroten @Wyverald
Make repo marker file parsing robust to version changes
6a417e9 
Bazel 7.1.0 and 7.2.0 contains a bug where + characters in labels in the repository marker files cannot be parsed. This was fixed in commit d62e0a0. To reduce the rusk of future bugs in the same area, this change skips parsing the file if the first line shows that the content will not be used anyway, which should be reasonably safe if introducing new formats.

Improves #23336 that fixed #23322.

Closes #23642.

PiperOrigin-RevId: 679330823
Change-Id: I8123bdde047735ced3eabed3df68112d44318b08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum approved these changes

@meteorcloudy meteorcloudy meteorcloudy approved these changes

@lberki lberki Awaiting requested review from lberki lberki is a code owner

Assignees
No one assigned
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash on tilde separators in cache
6 participants
@Wyverald @fmeum @moroten @meteorcloudy @sgowroji @iancha1992